Skip to content

Conversation

InvisibleProgrammer
Copy link
Contributor

@InvisibleProgrammer InvisibleProgrammer commented Oct 3, 2025

(cherry picked from commit 333227fbd13821365cec1bdbfcb9314a239bea0f)

Note: this is not a full port: it is about porting some missing changes from the previous port especially for test cases.

What changes were proposed in this pull request?

Add missing ports from Iceberg in TestHiveCommitLocks.

Why are the changes needed?

It lead to flakyness on downstream.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@deniskuzZ
Copy link
Member

deniskuzZ commented Oct 6, 2025

@InvisibleProgrammer since it's an addendum, I noticed that doUnlock was moved to MetastoreLock but wasn't removed from HiveTableOperations and is a dead code now.

void doUnlock(HiveLock lock) {
    if (lock != null) {
      try {
        lock.unlock();
      } catch (Exception e) {
        LOG.warn("Failed to unlock {}.{}", database, tableName, e);
      }
    }
  }

@deniskuzZ
Copy link
Member

could you please update the commit message, explicitly mentioning that we only port the test fixes, not the production code

…ableOperations (#6648)

Note: this is not a full port: it is about porting some missing changes from the previous port especially for test cases.

Co-authored-by: Adam Szita <[email protected]>
Co-authored-by: Peter Vary <[email protected]>
(cherry picked from commit 333227fbd13821365cec1bdbfcb9314a239bea0f)
@InvisibleProgrammer
Copy link
Contributor Author

Both commit message and PR description modified.
doUnlock removed from HiveTableOperations.

@deniskuzZ
Copy link
Member

Both commit message and PR description modified. doUnlock removed from HiveTableOperations.

thanks @InvisibleProgrammer, i'll merge with the green build

Copy link

sonarqubecloud bot commented Oct 6, 2025

@deniskuzZ deniskuzZ merged commit b115c85 into apache:master Oct 7, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants